Skip to content

Handle FuncCall nodes that represent first class callables #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

spawnia
Copy link
Contributor

@spawnia spawnia commented Apr 19, 2022

Stumbled upon this while working on a fix for #31. The original fix no longer applies, but I think this is still valuable to ensure PHP 8.1 compatibility.

@dbrekelmans
Copy link
Collaborator

@spawnia Could you resolve the conflict?

Also would this interact with #33? I'm thinking it might be best to merge this PR before #33, what do you think?

@spawnia spawnia force-pushed the handle-potential-first-class-callable branch from 33e3983 to abaeae5 Compare November 25, 2024 08:54
@spawnia
Copy link
Contributor Author

spawnia commented Nov 25, 2024

@dbrekelmans I rebased atop the latest changes. I think #33 and this can be applied independently from one another. The condition I put to check for first class callables should come before the condition added there.

@shish
Copy link
Collaborator

shish commented Feb 19, 2025

Hi all, new maintainer here, hoping to get this merged if it's still relevant. Coming in with little context, what does the PR do? Could it include a unit test showing what fails now, and succeeds after the patch is applied?

(There are merge conflicts and failing tests, and I daren't try to fix it myself without understanding the intention behind the code 😅 )

…first-class-callable

# Conflicts:
#	composer.json
#	src/Rules/UseSafeFunctionsRule.php
@spawnia
Copy link
Contributor Author

spawnia commented Feb 24, 2025

Thank you @shish for looking into this. I tried adding a failing test. I think first-class callables should also be recognized by this rule, such as json_encode(...).

@shish
Copy link
Collaborator

shish commented Feb 25, 2025

I have enabled the CI, but it looks like the added unit test is failing both before and after the changes to add support for it o.o;;

@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.54%. Comparing base (4e0c969) to head (9d145a4).
Report is 3 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master      #32      +/-   ##
============================================
+ Coverage     92.70%   94.54%   +1.83%     
- Complexity       40       46       +6     
============================================
  Files             7        8       +1     
  Lines            96      110      +14     
============================================
+ Hits             89      104      +15     
+ Misses            7        6       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@spawnia
Copy link
Contributor Author

spawnia commented Apr 8, 2025

Looks like PHPStan has introduced a custom node type for first-class callables from functions, see https://apiref.phpstan.org/2.1.x/PHPStan.Node.FunctionCallableNode.html.

@spawnia
Copy link
Contributor Author

spawnia commented Apr 8, 2025

@shish I hope you don't mind the restructuring of the tests. I found it hard to fit in the new tests in a logical and structured manner without it.

@shish shish merged commit 5f9795e into thecodingmachine:master Apr 9, 2025
5 checks passed
@spawnia spawnia deleted the handle-potential-first-class-callable branch April 10, 2025 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants